Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change function signature from rest.API to rest.Send. #19

Merged
merged 1 commit into from
Oct 31, 2017
Merged

Change function signature from rest.API to rest.Send. #19

merged 1 commit into from
Oct 31, 2017

Conversation

janczer
Copy link
Contributor

@janczer janczer commented Oct 2, 2017

According to #13.

@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Oct 2, 2017
@SendGridDX
Copy link

SendGridDX commented Oct 2, 2017 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@thinkingserious thinkingserious added difficulty: medium fix is medium in difficulty hacktoberfest labels Oct 2, 2017
@thinkingserious
Copy link
Contributor

HI @janczer,

So that we don't have a breaking change, can you please implement rest.API as a pass through function that simply calls rest.Send?

With Best Regards,

Elmer

@janczer janczer force-pushed the change_function_api_to_send branch from 891a214 to bc35a0f Compare October 3, 2017 05:44
@janczer
Copy link
Contributor Author

janczer commented Oct 3, 2017

Hi @thinkingserious

Sure. I added it and smash to the one commit.

thinkingserious
thinkingserious requested changes Oct 4, 2017
View reviewed changes
rest_test.go Outdated
@@ -95,7 +95,7 @@ func TestRest(t *testing.T) {
Headers: Headers,
QueryParams: queryParams,
}
response, e := API(request)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add these tests back in, in addition to the new ones you created for Send. Thanks!

Copy link
Contributor Author

@janczer janczer Oct 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added function testingAPI, and I'm sending to this function like parameter functions Send and API because code the same.

@janczer janczer force-pushed the change_function_api_to_send branch from bc35a0f to 84c163f Compare October 4, 2017 14:31
@thinkingserious thinkingserious mentioned this pull request Oct 12, 2017
Merged
@thinkingserious thinkingserious mentioned this pull request Oct 22, 2017
Closed
@thinkingserious
Copy link
Contributor

My apologies @janczer,

Could you please update the merge conflicts?

Thanks!

With Best Regards,

Elmer

@janczer
Copy link
Contributor Author

janczer commented Oct 31, 2017

@thinkingserious

I rebased my branche and merge conflicts.

Thanks!

@thinkingserious thinkingserious merged commit 811c319 into sendgrid:master Oct 31, 2017
@thinkingserious
Copy link
Contributor

Hello @janczer,

Thanks again for the PR!

It's HACKTOBERFEST! We want to show our appreciation by sending you some special Hacktoberfest swag. If you have not already, could you please fill out this form so we can send it to you? Thanks!

Team SendGrid DX

@jackgardner
Copy link

@janczer @thinkingserious

I think you are also missing the API() passthrough on the Client struct, or sendgrid-go also needs a PR:

https://github.com/sendgrid/sendgrid-go/blob/238cd04b958416fad59abdac707079c91d080bc9/sendgrid.go#L54

@thinkingserious
Copy link
Contributor

Yes, sendgrid-go will need an updated PR. That would be a most excellent hacktoberfest contribution ;)

This was referenced Oct 31, 2017
Closed
Closed
@janczer janczer deleted the change_function_api_to_send branch November 1, 2017 07:54
@jerbob92 jerbob92 mentioned this pull request Nov 1, 2017
Merged
6 tasks
@jerbob92
Copy link

jerbob92 commented Nov 1, 2017

Honestly, I do not appreciate breaking the package API.
I created a MR (#68) to also add fallback functionality for the API() function in the Client.

@thinkingserious
Copy link
Contributor

Thanks for catching this @jerbob92!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thinkingserious thinkingserious thinkingserious approved these changes

Assignees
No one assigned
Labels
difficulty: medium fix is medium in difficulty status: code review request requesting a community code review or review from Twilio
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@janczer @SendGridDX @thinkingserious @jackgardner @jerbob92